request interception #930
request interception #930
Conversation
There was a problem hiding this comment.
I know this is still in draft.
I like how little impact this has on the http/*.
Do you think it's possible to re-add the http_request_fail and http_request_complete? We could then merge this into nonblocking_libcurl and then into main, as these 2 (+http_request_start) are all that's missing.
My main concern, and what I got stuck on for while before ignoring it, is dealing with the start_callback. My understanding is that it can modify the headers and the body, but I'm not sure why it happens as a callback as opposed to when creating the Request.
I'm OK with both adding them here or taking the |
|
The only thing using the startCallback is XHR (ScriptManager is only using to log). I was going to say we can remove it..but... XHR does two things with this: 2 - it gets a reference to the transfer, which it needs in case xhr.abort() is called. The only solution I can think for this is that |
karlseguin
force-pushed
the
nonblocking_libcurl
branch
from
64f79f2 to
079ce5e
Compare
This ensures that page.wait won't unblock too early. As-is, this isn't an issue since active can only be 0 if there are no active OR pending requests. However, with request interception (#930) it's possible to have no active requests and no pending requests - from the http client's point of view - but still have pending-on-intercept requests. An alternative to this would be to undo these changes, and instead change Page.wait to be intercept-aware. That is, Page.wait would continue to block on http activity and scheduled tasks, as well as intercepted requests. However, since the Page doesn't know anything about CDP right now, and it does know about the http client, maybe doing this in the client is fine.
sjorsdonkers
force-pushed
the
request_interception
branch
from
910c715 to
44e951d
Compare
intercept continue and abort feedback First version of headers, no cookies yet
sjorsdonkers
force-pushed
the
request_interception
branch
from
44e951d to
03694b5
Compare
| } | ||
|
|
||
| fn parseHeader(header_str: []const u8) ?struct { name: []const u8, value: []const u8 } { | ||
| const colon_pos = std.mem.indexOf(u8, header_str, ":") orelse return null; |
There was a problem hiding this comment.
indexOfScalar removes 1 len check.
| @@ -467,12 +467,15 @@ pub const Page = struct { | |||
| const owned_url = try self.arena.dupeZ(u8, request_url); | |||
| self.url = try URL.parse(owned_url, null); | |||
|
|
|||
| var headers = try HttpClient.Headers.init(); | |||
There was a problem hiding this comment.
A lot of internal failure cases will result in this leaking. But I think we can leave it as-is for now.
The difficulty in resource management here is that we need to be careful about allocations up to the point where curl_multi_add_handle is called. After that, it's handled by the perform loop. So a simple errdefer won't do, since you risk double-free depending on where the failure is. A simple fix is to not callperformin makeRequest - making curl_multi_add_handle the last-called function - but I feel like delaying perform until the next tick (especially since there's no reason to think the request can't be sent out immediately) is unnecessary latency.
I want to address it, but, at this point, after we merge everything.
Depends on: #922
Also reenables:
http_request_startTODO:
Example playwright fragment: